-
Notifications
You must be signed in to change notification settings - Fork 13.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix multilayer geoviz and color picker error #5767
Fix multilayer geoviz and color picker error #5767
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5767 +/- ##
==========================================
+ Coverage 63.81% 63.81% +<.01%
==========================================
Files 364 364
Lines 23055 23080 +25
Branches 2568 2572 +4
==========================================
+ Hits 14713 14729 +16
- Misses 8327 8336 +9
Partials 15 15
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for catching all these little problems! There's just one modification we should do: before we were passing payload
, I changed it to pass only data
(breaking the generic interface), and now you've changed it to pass both. Let's pass only the payload, and extract data
from it.
let filters = subslice.form_data.filters.concat(fd.filters); | ||
let filters = []; | ||
if (subslice.form_data.filters) { | ||
filters = filters.concat(subslice.form_data.filters); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, can you use array spread here instead of concat
?
filters = [...filters, ...subslice.form_data.filters];
Same below, in the two uses of concat
. It's the new preferred way of concatenating arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got you. Thank!
} | ||
if (fd.filters) { | ||
filters = filters.concat(fd.filters); | ||
} | ||
if (fd.extra_filters) { | ||
filters = filters.concat(fd.extra_filters); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could do it in a single pass:
const filters = [
...(subslice.form_data.filters || [])
...(fd.filters || [])
...(fd.extra_filters || [])
];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah! thanks
|
||
return { | ||
'arcs': arcs, | ||
'features': d['features'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 I think it's good to standardize on the key value here.
} | ||
const c = fd.color_picker || { r: 0, g: 0, b: 0, a: 1 }; | ||
const color = [c.r, c.g, c.b, c.a * 255]; | ||
return { ...d, radius, color }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this, I think I must've removed it by mistake.
@@ -18,7 +18,8 @@ function getPoints(data) { | |||
return points; | |||
} | |||
|
|||
function getLayer(fd, data, slice) { | |||
function getLayer(fd, payload, slice) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I simplified the getLayer
interface but forgot that other deck.gl vizs need other attributes in the payload.
} | ||
|
||
ReactDOM.render( | ||
<CategoricalDeckGLContainer | ||
slice={slice} | ||
data={payload.data.arcs} | ||
data={payload.data.features} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're passing payload
below then we shouldn't pass data
.
@@ -54,6 +59,7 @@ function deckScatter(slice, payload, setControlValue) { | |||
setControlValue={setControlValue} | |||
viewport={viewport} | |||
getLayer={getLayer} | |||
payload={payload} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove data
above, since we're passing the payload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! make sense
@@ -35,6 +35,7 @@ const propTypes = { | |||
setControlValue: PropTypes.func.isRequired, | |||
viewport: PropTypes.object.isRequired, | |||
getLayer: PropTypes.func.isRequired, | |||
payload: PropTypes.object.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove data
from the props, since it can fetched from the payload.
@betodealmeida Comments addressed. PTAL |
339a8c9
to
fd85b97
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youngyjd looks great! I have just a small comment on the payload inside getLayer, we can change it so data doesn't get copied.
@@ -80,7 +80,7 @@ export default class CategoricalDeckGLContainer extends React.PureComponent { | |||
} | |||
getLayers(values) { | |||
const fd = this.props.slice.formData; | |||
let data = [...this.props.data]; | |||
let data = [...this.props.payload.data.features]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you're making a copy of this.props.payload.data.features
, mutating it, and then assigning it back to payload
in line 107. Here you can simply do:
let data = this.props.payload.data.features;
And then as you mutate it, payload.data.features
is also mutated. This way you can simply pass the original payload to getLayer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some test and found it does not work. I guess it is because the location that data
references got changed so payload.data.features
will not get updated.
This is part of the code:
let data = [...this.props.payload.data.features];
// Add colors from categories or fixed color
data = this.addColor(data, fd);
If we change it to let data = this.props.payload.data.features;
and then data
get reassigned by data = this.addColor(data, fd);
, it is not referencing to the location of this.props.payload.data.features
anymore so this.props.payload.data.features
will not be changed.
return [this.props.getLayer(fd, data, this.props.slice)]; | ||
const payload = this.props.payload; | ||
payload.data.features = data; | ||
return [this.props.getLayer(fd, payload, this.props.slice)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can simply be written as:
return [this.props.getLayer(fd, this.props.payload, this.props.slice)];
If you follow my comment above. You can also do in the beginning of the function:
const { getLayer, payload, slice } = this.props;
The you can simple refer to getLayer
, payload
and slice
inside the function, instead of this.props.getLayer
, this.props.payload
and this.props.slice
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of good educations here! Thanks!
The reason why I am making a copy of this.props.payload.data.features
, mutating it, and then assigning it back to payload in line 107 is that if I understand correctly, changing in this.props
will result in re-render. As we are only passing it to getLayer
function and don't need it to be re-rendered, I copy it so props
is unaffected and unnecessary re-render can be avoided. This is my understanding but I am not very sure whether it is correct based on your suggestion. Please correct me if I am wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well. I am wrong. Only state change will cause re-render.
@@ -40,17 +41,17 @@ function deckArc(slice, payload, setControlValue) { | |||
}; | |||
|
|||
if (fd.autozoom) { | |||
viewport = common.fitViewport(viewport, getPoints(payload.data.arcs)); | |||
viewport = common.fitViewport(viewport, getPoints(payload.data.features)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: you can use data
here instead of payload.data.features
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems not. data
is not defined in function deckArc(slice, payload, setControlValue){}
so I feel using data
here will not work. It is extracted in function getLayer(fd, payload, slice){}
instead.
function getLayer(fd, payload, slice) {
const data = payload.data.features;
const sc = fd.color_picker;
const tc = fd.target_color_picker;
return new ArcLayer({
id: `path-layer-${fd.slice_id}`,
data,
getSourceColor: d => d.sourceColor || d.color || [sc.r, sc.g, sc.b, 255 * sc.a],
getTargetColor: d => d.targetColor || d.color || [tc.r, tc.g, tc.b, 255 * tc.a],
strokeWidth: (fd.stroke_width) ? fd.stroke_width : 3,
...common.commonLayerProps(fd, slice),
});
}
function deckArc(slice, payload, setControlValue) {
const fd = slice.formData;
let viewport = {
...fd.viewport,
width: slice.width(),
height: slice.height(),
};
if (fd.autozoom) {
viewport = common.fitViewport(viewport, getPoints(payload.data.features));
}
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you're right. I got confused by the hidden lines, my bad!
Ready for another review @betodealmeida |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks! :)
@@ -40,17 +41,17 @@ function deckArc(slice, payload, setControlValue) { | |||
}; | |||
|
|||
if (fd.autozoom) { | |||
viewport = common.fitViewport(viewport, getPoints(payload.data.arcs)); | |||
viewport = common.fitViewport(viewport, getPoints(payload.data.features)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you're right. I got confused by the hidden lines, my bad!
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)
* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit
Several improvements and bug fixes in this PR.
multilayer geoviz
was unable to be rendered because queries did not filter out null locations by default. Changingif fd.get('filter_nulls')
toif fd.get('filter_nulls', True)
fixed the issue.getLayer()
function inscatter.jsx
is also called bymultilayer geoviz
directly. After introducing componentCategoricalDeckGLContainer
, thegetLayer()
interface got changed so scatterplot failed to be rendered inmultilayer geoviz
.color picker
does not work with scatter plot and it is always showing black point. This issue is fixed as well.